Conversation
- Convert 8 standalone error mapping tests into 2 parameterized rstests - Add descriptive case names (#[case::name]) with inline comments - Preserve 4 standalone tests for ToolOutput conversion scenarios - Net reduction: 6 lines (199→193)
- Convert 4 standalone test functions to single parameterized rstest - Add descriptive case names (#[case::name]) for clearer test output - Preserve all test scenarios with improved assertions - Use #[cfg_attr] for platform-specific test cases - Net reduction: 3 lines (36→33)
Replace 11 standalone test functions with two grouped rstests: - preprocess_transforms_to_block_scalar (4 cases for rewrite scenarios) - preprocess_preserves_unchanged (7 cases for identity scenarios) All 11 test scenarios preserved with descriptive case names and inline comments. Preserve-group assertions strengthened from partial contains to exact equality.
Convert 3 standalone test functions to single parameterized rstest: - sorts_alphabetically (was render_task_targets_sorts_output_by_name) - shows_name_and_description (was render_task_targets_shows_only_name_and_description) - handles_empty_input (was render_task_targets_handles_empty_input_cleanly) - Add rstest import to test module - Preserve all test coverage with stronger exact-equality assertions - Keep task_tool_definition test as separate standalone test - Net reduction: 6 lines (36→30)
Convert matches_agent_pattern_works (7 assertions) and derive_agent_name_from_rel_works (6 assertions) to parameterized rstest cases: - Add descriptive case names (#[case::name]) with inline comments - All 13 original assertion scenarios preserved - Net reduction: 7 lines (47→40) - All tests pass, clippy clean, verify.sh passes
Convert 16 standalone test functions to 4 parameterized rstests: - parse_agent_success_cases (8 named cases for valid parsing) - parse_agent_error_cases (4 named cases using ExpectedParseError enum) - parse_agent_crlf_normalization (2 named cases for CRLF handling) - parse_agent_permission_validation (2 named cases for permission rejection) Keep parse_error_display_messages as standalone test. Net reduction: 12 lines (503→491).
- Add rstest = "0.26" to models-dev dev-dependencies - Convert provider_type_mapping_handles_known_and_unknown_packages from 17 sequential assertions to parameterized #[rstest] with 17 named cases - Convert all_known_provider_types_round_trip from loop over 16 types to parameterized #[rstest] with 16 named cases - Update Cargo.lock accordingly
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
=======================================
Coverage 78.36% 78.36%
=======================================
Files 108 108
Lines 3942 3942
=======================================
Hits 3089 3089
Misses 853 853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.rs📄 CodeRabbit inference engine (src/AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-03-17T21:06:53.934ZApplied to files:
🔇 Additional comments (2)
WalkthroughThis PR refactors many crate test modules to use parameterized Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/llm-coding-tools-models-dev/src/api/catalog_sources.rs (1)
546-587: Consider adding a test case for unknown npm package strings.The test covers
Nonemapping toProviderType::Unknown, but the function also handlesSome(_)(unknown package names) in the same match arm. Adding a case like#[case::unknown_package(Some("@unknown/package"), ProviderType::Unknown)]would explicitly verify the catch-all behavior.💡 Optional: Add unknown package case
#[case::antigravity_package(Some("@ai-sdk/antigravity"), ProviderType::Antigravity)] #[case::missing_package_unknown(None, ProviderType::Unknown)] +#[case::unrecognized_package_unknown(Some("@unknown/package"), ProviderType::Unknown)] fn npm_package_maps_to_correct_provider_type(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-models-dev/src/api/catalog_sources.rs` around lines 546 - 587, Add an explicit test case for unknown npm package strings in the existing parameterized test npm_package_maps_to_correct_provider_type: include a new rstest case (e.g., #[case::unknown_package(Some("@unknown/package"), ProviderType::Unknown)]) so that provider_type_from_models_dev_npm(Some(_)) unknown values are asserted to map to ProviderType::Unknown in addition to the existing None case.src/llm-coding-tools-agents/src/parser/mod.rs (1)
357-378: Asserthiddenin every success case to avoid silent coverage loss.At Line 376,
hiddenis only checked whenexpected_hiddenisSome(_), so most success cases don’t validate the default/derived value.♻️ Proposed change
- #[case] expected_hidden: Option<bool>, + #[case] expected_hidden: bool, @@ - if let Some(expected_hidden) = expected_hidden { - assert_eq!(result.data.hidden, expected_hidden); - } + assert_eq!(result.data.hidden, expected_hidden);- false, "Prompt body here.", "Test agent", None, None + false, "Prompt body here.", "Test agent", None, false @@ - false, "indented\n\ntrailing", "Test", None, None + false, "indented\n\ntrailing", "Test", None, false @@ - false, "🙂 café 漢字", "Test", None, None + false, "🙂 café 漢字", "Test", None, false @@ - false, "", "Test", None, None + false, "", "Test", None, false @@ - true, "body", "Test", None, None + true, "body", "Test", None, false @@ - false, "body", "Test", Some("provider/model:tag"), None + false, "body", "Test", Some("provider/model:tag"), false @@ - false, "body", "Test", None, None + false, "body", "Test", None, false @@ - false, "body", "Test", None, Some(true) + false, "body", "Test", None, true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-agents/src/parser/mod.rs` around lines 357 - 378, The test parse_agent_success_cases currently only asserts result.data.hidden when expected_hidden is Some(...), leaving many cases unchecked; update the function to always assert the hidden value by replacing the conditional check with a single assertion that compares result.data.hidden to expected_hidden.unwrap_or(false) (or alternatively change the parameter to a plain bool and update callers), referencing parse_agent_success_cases, expected_hidden and result.data.hidden so every success case validates the hidden field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-serdesai/src/convert.rs`:
- Around line 165-175: The test currently moves serdes_err by using
matches!(serdes_err, SerdesError::ValidationFailed { .. }) which causes a
move-before-use for later calls to serdes_err.message(); update the pattern to
match by reference (e.g. matches!(&serdes_err, SerdesError::ValidationFailed {
.. })) so serdes_err is not moved and can be used in the subsequent
expected_msg/expected_extra checks; adjust the matches invocation in the
function/ test where serdes_err is defined to use a borrowed match.
---
Nitpick comments:
In `@src/llm-coding-tools-agents/src/parser/mod.rs`:
- Around line 357-378: The test parse_agent_success_cases currently only asserts
result.data.hidden when expected_hidden is Some(...), leaving many cases
unchecked; update the function to always assert the hidden value by replacing
the conditional check with a single assertion that compares result.data.hidden
to expected_hidden.unwrap_or(false) (or alternatively change the parameter to a
plain bool and update callers), referencing parse_agent_success_cases,
expected_hidden and result.data.hidden so every success case validates the
hidden field.
In `@src/llm-coding-tools-models-dev/src/api/catalog_sources.rs`:
- Around line 546-587: Add an explicit test case for unknown npm package strings
in the existing parameterized test npm_package_maps_to_correct_provider_type:
include a new rstest case (e.g.,
#[case::unknown_package(Some("@unknown/package"), ProviderType::Unknown)]) so
that provider_type_from_models_dev_npm(Some(_)) unknown values are asserted to
map to ProviderType::Unknown in addition to the existing None case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a909db14-6fa8-42f4-9f66-e0578349da81
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/llm-coding-tools-agents/src/loader.rssrc/llm-coding-tools-agents/src/parser/mod.rssrc/llm-coding-tools-agents/src/parser/preprocessor.rssrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-core/src/path/absolute.rssrc/llm-coding-tools-models-dev/Cargo.tomlsrc/llm-coding-tools-models-dev/src/api/catalog_sources.rssrc/llm-coding-tools-models-dev/src/cache/payload.rssrc/llm-coding-tools-serdesai/src/convert.rssrc/llm-coding-tools-serdesai/src/task/definition.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Async Windows
- GitHub Check: Blocking Windows
- GitHub Check: Blocking Linux
- GitHub Check: Async Linux
- GitHub Check: Async macOS
- GitHub Check: Blocking macOS
🧰 Additional context used
📓 Path-based instructions (2)
src/**/Cargo.toml
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/Cargo.toml: Enabletokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error.
Prefer performance-oriented crates such asparking_lotoverstd::syncandmemchrfor byte search
Keep dependency footprint minimal
Files:
src/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-models-dev/Cargo.toml
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/path/absolute.rssrc/llm-coding-tools-models-dev/src/api/catalog_sources.rssrc/llm-coding-tools-models-dev/src/cache/payload.rssrc/llm-coding-tools-serdesai/src/task/definition.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-agents/src/loader.rssrc/llm-coding-tools-agents/src/parser/preprocessor.rssrc/llm-coding-tools-serdesai/src/convert.rssrc/llm-coding-tools-agents/src/parser/mod.rs
🧠 Learnings (9)
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
src/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-models-dev/Cargo.toml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
src/llm-coding-tools-models-dev/Cargo.toml
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/path/absolute.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Prefer `core` over `std` where possible (e.g., `core::mem` over `std::mem`)
Applied to files:
src/llm-coding-tools-core/src/path/absolute.rs
📚 Learning: 2026-03-11T22:12:27.804Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 54
File: src/llm-coding-tools-models-dev/src/catalog/load_cache.rs:23-29
Timestamp: 2026-03-11T22:12:27.804Z
Learning: In `src/llm-coding-tools-models-dev/src/cache/` (Rust, llm-coding-tools-models-dev crate), the on-disk cache (models.dev.catalog.v1.cache) is assumed to be written only by the local user/process. Malicious or externally-crafted cache files are explicitly out of scope for this threat model, so there is no need to add upper-bound validation on `payload_len_decompressed` before calling `zstd::bulk::decompress`.
Applied to files:
src/llm-coding-tools-models-dev/src/cache/payload.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-serdesai/src/task/definition.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-agents/src/parser/preprocessor.rs
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In the `src/llm-coding-tools-bubblewrap/src/profile/` module (Rust, `llm-coding-tools-bubblewrap` crate), reviewers should assume the `Builder` API’s path fields (`workspace`, `synthetic_home`, `cache_root`, mount lists, overlays, etc.) are set only by trusted application/operator code. Therefore, absence of traversal hardening/normalization in validator helpers (e.g., `validate_absolute_path`) should not be treated as a traversal vulnerability by itself.
However, this applies only if the call graph guarantees that untrusted/LLM-generated shell inputs never reach these `Builder` path fields. Untrusted input is expected to enter only via `wrap_command` / `execute_command_with_mode`. During review, verify the trust boundary: if any LLM/untrusted data can influence the `Builder` path fields or mount/overlay sources, then path normalization/`..`-component hardening and traversal-safe checks are required and should be flagged.
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/types.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Place `use` statements inside functions only for `#[cfg]` conditional compilation
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/types.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Use `Cow<'_, str>` for conditional ownership (e.g., `String::from_utf8_lossy`)
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/types.rs
🔇 Additional comments (14)
src/llm-coding-tools-models-dev/Cargo.toml (1)
67-67: LGTM!The
rstest = "0.26"dev-dependency addition is appropriate for enabling parameterized tests in this crate.src/llm-coding-tools-bubblewrap/Cargo.toml (1)
22-22: LGTM!Consistent
rstest = "0.26"dev-dependency addition matching other crates in this PR.src/llm-coding-tools-models-dev/src/cache/payload.rs (1)
235-273: LGTM!The parameterized test correctly covers all 16
ProviderTypevariants from the enum definition. Each case verifies the serialization/deserialization round-trip through the cache payload, and the doc comment clearly describes the test's purpose.src/llm-coding-tools-bubblewrap/src/profile/types.rs (1)
565-606: LGTM!The parameterized test correctly verifies
is_prevalidated_workdirbehavior: exact matches against all prevalidated roots (workspace, synthetic home, cache root, bind-mounted tmp, read-only/read-write mounts) returntrue, while nested paths and unmounted paths correctly returnfalse. The fixture setup aligns perfectly with the test cases.src/llm-coding-tools-core/src/path/absolute.rs (1)
45-93: LGTM!The parameterized test correctly uses
#[cfg_attr]for platform-conditional cases, testing both absolute path acceptance and relative path rejection. The error validation properly checks for theToolError::InvalidPathvariant and verifies the error message content.src/llm-coding-tools-agents/src/loader.rs (1)
418-442: LGTM!Both parameterized tests correctly cover the pattern matching and name extraction logic. The
agent_path_pattern_matchingtest verifies directory prefix and extension requirements, whileextracts_agent_name_from_rel_pathcorrectly tests prefix stripping, nested path preservation, and empty-stem rejection.src/llm-coding-tools-agents/src/parser/preprocessor.rs (1)
245-337: LGTM!The two parameterized test suites cleanly separate transformation cases from preservation cases.
preprocess_transforms_to_block_scalarcorrectly verifies block scalar conversion with optional CRLF normalization and inline comment stripping, whilepreprocess_preserves_unchangedensures idempotency for already-safe YAML constructs. The use offorbidden_fragmentto verify comment removal is a nice touch.src/llm-coding-tools-agents/src/parser/mod.rs (5)
253-261: Good foundation for parameterized error-path testing.The
ExpectedParseErrorenum cleanly decouples case inputs from assertion logic and keeps the test matrix readable.
381-426: Error-case consolidation looks solid.Variant-level matching plus schema-message fragment checks are a good balance of precision and stability.
428-443: CRLF normalization cases are well-structured.Using explicit
\r\nliterals here is the right choice and avoidsindoc!normalization side effects.
445-475: Permission validation scenarios are well covered.Covering both scalar and pattern-map forms for
permission.task: askkeeps this guardrail robust.
477-495: Display-message parameterization is clear and precise.Exact string assertions for
AgentParseError::to_string()are straightforward and maintainable.src/llm-coding-tools-serdesai/src/convert.rs (1)
91-134: Good test consolidation with preserved assertion strength.The
rstestparameterization keeps the original scenarios while maintaining meaningful checks (variant mapping,tool_name, and message content).src/llm-coding-tools-serdesai/src/task/definition.rs (1)
76-114: Nicerstestmigration for render-target coverage.The cases are clear and robust (exact rendered output, ordering, forbidden fragment check, and empty-state fallback).
…ping - Always assert hidden field value in parse_agent_success_cases using unwrap_or(false) default - Add test case for unknown npm package mapping to ProviderType::Unknown
1 participant
Summary
This PR refactors tests across the workspace to use the `rstest` crate for parameterized testing, reducing code duplication and improving maintainability.
Changes
Benefits
Testing
All existing test cases preserved, now expressed as parameterized rstest cases.